-
Notifications
You must be signed in to change notification settings - Fork 2.2k
feat: add support for eth/70 eip-7542 #20133
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
mattsse
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is awesome, only have a few requests
| /// The block range this node can serve (eth/70). | ||
| pub block_range: Option<BlockRange>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we dont technically need this, but I think we can be exhaustive here and have all versioned fields, so let's keep this
| pub const LATEST: Self = Self::Eth70; | ||
|
|
||
| /// All known eth versions | ||
| pub const ALL_VERSIONS: &'static [Self] = &[Self::Eth69, Self::Eth68, Self::Eth67, Self::Eth66]; | ||
| pub const ALL_VERSIONS: &'static [Self] = | ||
| &[Self::Eth70, Self::Eth69, Self::Eth68, Self::Eth67, Self::Eth66]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to keep these unchanged for now, we can enable this once we're ready to enable full support for this
| if let StatusMessage::Eth70(s) = their_status_message { | ||
| if !s.block_range.is_valid() { | ||
| return Err(EthHandshakeError::EarliestBlockGreaterThanLatestBlock { | ||
| got: s.block_range.start_block, | ||
| latest: s.block_range.end_block, | ||
| } | ||
| .into()); | ||
| } | ||
|
|
||
| if s.blockhash.is_zero() { | ||
| return Err(EthHandshakeError::BlockhashZero.into()); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great
| /// | ||
| /// Returns an error if the message is considered to be in violation of the protocol. | ||
| fn on_incoming_message(&mut self, msg: EthMessage<N>) -> OnIncomingMessageOutcome<N> { | ||
| let now = Instant::now(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can avoid this here and instead only instantiate this when we need to record this based on the message variant
| // If our view of the peer's range is stale, request an update (throttled). | ||
| let now = Instant::now(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a bit too expensive,
can we combine this with this interval here:
reth/crates/net/network/src/session/active.rs
Lines 737 to 739 in 1902970
| if let Some(interval) = &mut this.range_update_interval { | |
| // Check if we should send a range update based on block height changes | |
| while interval.poll_tick(cx).is_ready() { |
| interval.set_missed_tick_behavior(tokio::time::MissedTickBehavior::Delay); | ||
| interval | ||
| }); | ||
| let now = Instant::now(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can be moved into the closure, then we dont always call this
mattsse
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm very sorry https://eips.ethereum.org/EIPS/eip-7542 was withdrawn and instead https://eips.ethereum.org/EIPS/eip-7975 will become eth70...
for this pr this means we need to remove the additional message variants and instead introduce a Receipts70 variant, similar to how receipts69 works I believe
This targets on issue #19805 , which i did below :
Added eth/70 wire support: new StatusEth70 with block range, BlockRange type, and RequestBlockRange/SendBlockRange messages (IDs 0x0b/0x0c);
Extended capability/version helpers to include eth/70 alongside eth/69; kept eth/69 compatibility tests and added eth/70 defaults.
Handshake/status conversion now carries block range, validates eth/70 ranges and nonzero blockhash, and maps unified status to the proper message variant.
eth/70 peers send SendBlockRange instead of BlockRangeUpdate, handle incoming range requests/responses, update remote range info, and throttle periodic range requests when info is stale
Add eth/69-like tests
cc @mattsse